Skip to content

HDDS-14730. Update Recon container sync to use container IDs#9842

Merged
devmadhuu merged 13 commits intoapache:masterfrom
jasonosullivan34:HDDS-14730-container-ids
Mar 25, 2026
Merged

HDDS-14730. Update Recon container sync to use container IDs#9842
devmadhuu merged 13 commits intoapache:masterfrom
jasonosullivan34:HDDS-14730-container-ids

Conversation

@jasonosullivan34
Copy link
Contributor

@jasonosullivan34 jasonosullivan34 commented Feb 27, 2026

What changes were proposed in this pull request?

  • Change to use Container IDs to reduce the payload size for the RPCs between Recon and SCM for container sync

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-14730

How was this patch tested?

Unit tests
Manual testing

@devmadhuu

@devmadhuu devmadhuu self-requested a review February 27, 2026 11:43
Copy link
Contributor

@devmadhuu devmadhuu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jasonosullivan34 for the patch. Few comments in line with code. Pls check.

Also please add in PR description , how the patch was tested. Also better write following tests:

  • A unit test for ContainerStateMap.getContainerIDs(state, start, count) verifying pagination and state filtering

  • A unit or integration test for syncWithSCMContainerInfo() covering the "container missing from Recon, add it" path, and the "container already present, skip it" path

Copy link
Contributor

@devmadhuu devmadhuu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jasonosullivan34 for improving the patch, however, still few points need to handle. Pls check.

return true;
}

private long getContainerCountPerCall(long totalContainerCount) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Earlier, CONTAINER_METADATA_SIZE was defined as 1MB to estimate a ContainerInfo object. A ContainerID proto is ~8–12 bytes. With an IPC max of 128MB, the old code limited batches to floor(128MB / 1MB) = 128 containers per call. The new code does the same — 128 IDs per call — when it could safely fetch floor(128MB / 12 bytes) ≈ 5.5 million IDs per call. This means the change may actually increase the number of RPCs instead of reducing them. So I think, we should test with large set of container Ids specially when a cluster will have 4-5 millions CLOSED containers and record the SCM latency and impact. Because that was the whole objective behind why this JIRA was raised. And based on impact, we should think of innovative way to handle impact on SCM as well as rpc message length also should not exceed default 128 MB.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose limiting the number of container IDs we fetch back in a single message to 500K. This would equate to 16MB which is well within the RPC message size limit and should be safe enough for SCM to handle.

I also want to introduce a config where we can reduce the number of container ids fetched if we need to fine tune due to memory constraints

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "~16MB heap" is computed as 500K × ~32 bytes per JVM ContainerID object. But the wire size is 500K × 12 bytes = 6MB. Check my comment in other place.

@jasonosullivan34 jasonosullivan34 marked this pull request as ready for review March 20, 2026 15:09
Copy link
Contributor

@devmadhuu devmadhuu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasonosullivan34 thanks for improving the patch. Just few nits. Now it LGTM.

* Maximum number of ContainerIDs to fetch from SCM per RPC call
* during container sync. Each ContainerID is approximately 12 bytes
* on the wire. Reduce this value on memory-constrained Recon nodes.
* Default: 500,000 (~16MB heap per batch, 8 calls for a 4M container cluster)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "~16MB heap" is computed as 500K × ~32 bytes per JVM ContainerID object. But the wire size is 500K × 12 bytes = 6MB. These are different things and the comment doesn't distinguish them. Operators tuning this config for RPC limits care about wire size; operators tuning for memory care about heap size. Clarify both: e.g., "~6MB on the wire, ~16MB JVM heap per batch". So based on wire size, number of containers in single rpc call also can be more in default ?

return true;
}

private long getContainerCountPerCall(long totalContainerCount) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "~16MB heap" is computed as 500K × ~32 bytes per JVM ContainerID object. But the wire size is 500K × 12 bytes = 6MB. Check my comment in other place.

… to use ContainerManager.getContainerStateCount(state)
Copy link
Contributor

@devmadhuu devmadhuu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jasonosullivan34 for improving the patch. LGTM +1

@devmadhuu devmadhuu merged commit 7ee78a2 into apache:master Mar 25, 2026
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants